feat: support custom data dir and log directories#302
Open
JackZhao10086 wants to merge 5 commits intomainfrom
Open
feat: support custom data dir and log directories#302JackZhao10086 wants to merge 5 commits intomainfrom
JackZhao10086 wants to merge 5 commits intomainfrom
Conversation
github-actions
bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughauthLogDir() and StorageDir() now prefer env vars ( Changes
Sequence Diagram(s)(omitted — changes are localized path-validation and fallback logic, not multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/auth_log.go`:
- Around line 24-26: The returned LARKSUITE_CLI_LOG_DIR value is used later by
vfs.MkdirAll and vfs.OpenFile and must be validated; update the function in
auth_log.go that reads os.Getenv("LARKSUITE_CLI_LOG_DIR") to call
validate.SafeInputPath (or the project’s equivalent) on the value and only
return it if validation succeeds, otherwise fall back to the existing default
path or an empty string and log/handle the invalid input; ensure the validation
happens before any use in vfs.MkdirAll or vfs.OpenFile so the path cannot be
used without passing validate.SafeInputPath.
In `@internal/keychain/keychain_other.go`:
- Around line 28-29: The function currently returns the raw
LARKSUITE_CLI_DATA_DIR env value (os.Getenv("LARKSUITE_CLI_DATA_DIR")) without
appending the service name, causing data/credential collisions; change the
branch in StorageDir to return filepath.Join(dir, service) instead of dir so the
environment override preserves per-service isolation (mirror the default branch
which uses filepath.Join(xdgData, service)).
- Around line 28-30: The code returns the LARKSUITE_CLI_DATA_DIR env value
directly; validate this user-controlled path with validate.SafeInputPath before
returning/using it to prevent path traversal or malformed paths. Modify the
function that reads os.Getenv("LARKSUITE_CLI_DATA_DIR") to call
validate.SafeInputPath(dir) and handle validation errors (fallback to default
dir or return an error) so all subsequent uses (the functions reading files in
this package) receive a validated path.
- Around line 28-30: The macOS StorageDir implementation (StorageDir in
keychain_darwin.go) is missing the LARKSUITE_CLI_DATA_DIR environment override
present in the other implementation (keychain_other.go); add the same env check
at the start of StorageDir in keychain_darwin.go to return that dir if set (or,
alternatively, add a comment in StorageDir documenting that the override is
intentionally Linux-only). Update the StorageDir function in keychain_darwin.go
to mirror the logic: read os.Getenv("LARKSUITE_CLI_DATA_DIR") and return it when
non-empty before falling back to the existing macOS directory resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00e3fb58-540e-40ac-8609-d64e0fdb7d01
📒 Files selected for processing (2)
internal/keychain/auth_log.gointernal/keychain/keychain_other.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d009cba7e9ed0fae6a3831e3f9276256ee3c862a🧩 Skill updatenpx skills add larksuite/cli#feat/linux_env_data_dir -y -g |
Greptile SummaryThis PR adds two environment variable overrides — Confidence Score: 5/5Safe to merge; the only finding is a P2 usability gap (missing warning on invalid env var) that does not affect correctness or data integrity. The previous P1 concern about the service parameter being dropped is fixed. All remaining findings are P2 or lower — the silent fallback is a polish issue, not a correctness bug. internal/keychain/auth_log.go and internal/keychain/keychain_other.go — both are missing a stderr warning when env var validation fails.
|
| Filename | Overview |
|---|---|
| internal/keychain/auth_log.go | Adds LARKSUITE_CLI_LOG_DIR override with path validation; silently falls back on invalid values without emitting a warning as described in the PR. |
| internal/keychain/keychain_other.go | Adds LARKSUITE_CLI_DATA_DIR override with path validation and correct service-scoped sub-directory; same silent fallback issue as auth_log.go. |
| internal/validate/path.go | Adds SafeEnvDirPath: requires absolute path, rejects control chars, cleans/resolves via resolveNearestAncestor — correct and well-structured. |
| internal/keychain/auth_log_test.go | New tests cover valid env var usage and fallback on relative (invalid) paths; no build tag needed since auth_log.go is platform-agnostic. |
| internal/keychain/keychain_other_test.go | New linux-gated tests cover valid env var with service isolation and fallback to default XDG path; correct use of build constraint. |
| internal/validate/path_test.go | Adds tests for SafeEnvDirPath: covers relative-path rejection and canonical normalization of paths with dot-dot segments. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[authLogDir or StorageDir called] --> B{Custom env var set?}
B -- Yes --> C[SafeEnvDirPath validate]
C -- Valid --> D[Return custom dir plus service suffix]
C -- Invalid --> E[Silent fallback, no warning emitted]
B -- No --> F{LARKSUITE_CLI_CONFIG_DIR set?}
E --> F
F -- Yes --> G[Return CONFIG_DIR/logs]
F -- No --> H[UserHomeDir]
H -- ok --> I[Return default home-based path]
H -- err --> J[Print warning to stderr and return empty-home path]
Reviews (3): Last reviewed commit: "refactor(keychain): remove warning logs ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Add validation for environment variable directory paths to ensure they are absolute and safe. This prevents potential security issues from malformed paths. Also add corresponding tests to verify the validation behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/keychain/keychain_other_test.go (1)
10-20: Consider isolating HOME to prevent test flakiness.Similar to the auth_log test, if
LARKSUITE_CLI_DATA_DIRvalidation were to fail unexpectedly, this test would fall through to the HOME-based default rather than failing clearly. Isolating HOME ensures the test validates the intended code path.🔧 Suggested improvement
func TestStorageDir_UsesValidatedDataDirEnv(t *testing.T) { base := t.TempDir() base, _ = filepath.EvalSymlinks(base) t.Setenv("LARKSUITE_CLI_DATA_DIR", filepath.Join(base, "data", "..", "store")) + t.Setenv("HOME", t.TempDir()) // Isolate from home fallback got := StorageDir("svc")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_other_test.go` around lines 10 - 20, TestStorageDir_UsesValidatedDataDirEnv can silently pass if the LARKSUITE_CLI_DATA_DIR validation fails because the code falls back to HOME; to avoid flakiness, in the test (TestStorageDir_UsesValidatedDataDirEnv) explicitly isolate HOME (e.g., set HOME to a fresh temp directory or clear it via t.Setenv("HOME", ...) / t.Setenv("HOME", "") ) before calling StorageDir("svc") so the test cannot accidentally succeed by using the HOME-based default, ensuring the code path that reads and validates LARKSUITE_CLI_DATA_DIR is exercised.internal/keychain/auth_log_test.go (1)
8-19: Consider isolating HOME to prevent test flakiness.The test correctly validates the LOG_DIR path, but it doesn't isolate
HOME. IfLARKSUITE_CLI_LOG_DIRvalidation were to fail unexpectedly, the test would fall through to the home-based default (${HOME}/.lark-cli/logs) rather than failing clearly.🔧 Suggested improvement
func TestAuthLogDir_UsesValidatedLogDirEnv(t *testing.T) { base := t.TempDir() base, _ = filepath.EvalSymlinks(base) t.Setenv("LARKSUITE_CLI_LOG_DIR", filepath.Join(base, "logs", "..", "auth")) t.Setenv("LARKSUITE_CLI_CONFIG_DIR", "") + t.Setenv("HOME", t.TempDir()) // Isolate from home fallback got := authLogDir()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/auth_log_test.go` around lines 8 - 19, TestAuthLogDir_UsesValidatedLogDirEnv can fall back to the real HOME if LARKSUITE_CLI_LOG_DIR validation fails, making the test flaky; update the test to isolate HOME by setting a controlled value (e.g., t.Setenv("HOME", filepath.Join(base, "nohome")) or another t.TempDir()) before calling authLogDir(), so authLogDir() cannot accidentally read the real user home; modify the test body around the existing env setup (LARKSUITE_CLI_LOG_DIR, LARKSUITE_CLI_CONFIG_DIR) to also set HOME to the temporary directory to ensure deterministic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/keychain/auth_log_test.go`:
- Around line 8-19: TestAuthLogDir_UsesValidatedLogDirEnv can fall back to the
real HOME if LARKSUITE_CLI_LOG_DIR validation fails, making the test flaky;
update the test to isolate HOME by setting a controlled value (e.g.,
t.Setenv("HOME", filepath.Join(base, "nohome")) or another t.TempDir()) before
calling authLogDir(), so authLogDir() cannot accidentally read the real user
home; modify the test body around the existing env setup (LARKSUITE_CLI_LOG_DIR,
LARKSUITE_CLI_CONFIG_DIR) to also set HOME to the temporary directory to ensure
deterministic behavior.
In `@internal/keychain/keychain_other_test.go`:
- Around line 10-20: TestStorageDir_UsesValidatedDataDirEnv can silently pass if
the LARKSUITE_CLI_DATA_DIR validation fails because the code falls back to HOME;
to avoid flakiness, in the test (TestStorageDir_UsesValidatedDataDirEnv)
explicitly isolate HOME (e.g., set HOME to a fresh temp directory or clear it
via t.Setenv("HOME", ...) / t.Setenv("HOME", "") ) before calling
StorageDir("svc") so the test cannot accidentally succeed by using the
HOME-based default, ensuring the code path that reads and validates
LARKSUITE_CLI_DATA_DIR is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef73e922-76e1-4c3c-94b3-9ce91e3ac652
📒 Files selected for processing (6)
internal/keychain/auth_log.gointernal/keychain/auth_log_test.gointernal/keychain/keychain_other.gointernal/keychain/keychain_other_test.gointernal/validate/path.gointernal/validate/path_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/keychain/auth_log.go
- internal/keychain/keychain_other.go
Add missing documentation comments for SafeEnvDirPath function and related test cases to improve code clarity and maintainability
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds environment variable overrides for keychain-related local storage paths. This lets users customize the Linux keychain data directory and the auth log directory without changing the default behavior.
Changes
LARKSUITE_CLI_DATA_DIRsupport ininternal/keychain/keychain_other.goso Linux keychain storage can use a custom directoryLARKSUITE_CLI_LOG_DIRsupport ininternal/keychain/auth_log.goso auth logs can be written to a custom directoryTest Plan
lark xxxcommands locallyExecuted:
go test ./internal/keychainGOOS=linux GOARCH=amd64 go test -c -o /tmp/keychain_linux.test ./internal/keychainRelated Issues
Summary by CodeRabbit